Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a minimal xilem_masonry #205

Merged
merged 12 commits into from
Apr 25, 2024
Merged

Conversation

DJMcNab
Copy link
Member

@DJMcNab DJMcNab commented Apr 24, 2024

Alternative to #204

Demonstrates a viable path forward to Xilem. This does not get rid of the widget set in the main Xilem crate, or migrate the main Xilem crate to this

@DJMcNab DJMcNab marked this pull request as ready for review April 25, 2024 09:01
}
}

impl<State, Logic, View> Xilem<State, Logic, View>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be too cute, but I'd like to keep it like this

I wanted to avoid the term App, because this is plausibly embeddable.

///
/// This includes only the widgets which might send actions
/// This is currently never cleaned up
widget_map: HashMap<WidgetId, Vec<ViewId>>,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we expect Masonry to do internal tree structure tracking at some point, which I think would include widget deleted events?

I don't know a clean way to tidy this up, otherwise

///
/// These need to be public for type inference
#[doc(hidden)]
pub struct WasAView;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This avoids needing ViewMarker

}
}

// TODO: We use raw indexing for this value. What would make it invalid?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design of ViewSequence here is subtly different to previous versions of Xilem.

Currently, each non-trivial ViewSequences add a level to the ViewId path, and they have complete control of their own ViewId.

This is a quite different, much sparser model of view ids

}

fn mutate(&mut self) -> WidgetMut<Box<dyn masonry::Widget>> {
unreachable!("VecSplice can only be used for `build`, not rebuild")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an interesting one. I suspect that ViewSequence::build might want to take an append only Vec, and so only mutate takes an ElementSplice.


use crate::{ElementSplice, MasonryView, VecSplice, ViewSequence};

// TODO: Allow configuring flex properties. I think this actually needs its own view trait?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These kinds of specialised view traits would also be the idiomatic way to create and manage the properties of winit Windows, for example (in addition to menus?)

Copy link
Member Author

@DJMcNab DJMcNab Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cargo run --package xilem_masonry --example mason
xilem_masonry_flex.webm

@@ -105,4 +105,5 @@ jobs:
uses: Swatinem/rust-cache@v2

- name: cargo doc
run: cargo doc --workspace --all-features --no-deps --document-private-items -Zunstable-options -Zrustdoc-scrape-examples
# We currently skip checking masonry's docs
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're probably going to need to make an issue for this

Copy link
Contributor

@PoignardAzur PoignardAzur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

crates/xilem_masonry/src/sequence.rs Outdated Show resolved Hide resolved
}
}

const BASE_ID: NonZeroU64 = match NonZeroU64::new(1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be unwrap, I think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unwrap isn't const.

ViewId being NonZero is a little bit weird at the moment, so it might be worth reconsidering.

crates/xilem_masonry/src/sequence.rs Outdated Show resolved Hide resolved
@DJMcNab DJMcNab added this pull request to the merge queue Apr 25, 2024
Merged via the queue into linebender:main with commit 69ac080 Apr 25, 2024
7 checks passed
@DJMcNab DJMcNab deleted the xilem_masonry branch April 25, 2024 12:35
github-merge-queue bot pushed a commit that referenced this pull request Apr 26, 2024
Alternative to #218

The version of viewsequence added in #205 is robust for all properties
supported in that version. However, it would be incompatible with future
expansions to the Xilem model, in particular async wiring.

In this PR, I propose a system to resolve this, which uses generational
indexes in the view id path (for the items where this is relevant).

Some other historical record: #9

This brings back the `View::State` associated type (renamed to
`ViewState` and `SeqState` depending on the trait), in order to perform
this wiring.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants